Skip to content

Feat : Add Table of Contents Component on pages #1775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 42 commits into from
Mar 1, 2025

Conversation

ShubhamOulkar
Copy link
Member

@ShubhamOulkar ShubhamOulkar commented Feb 7, 2025

task

  • design table of content (TOC) component
  • responsive TOC btn and sub menu list
  • TOC dark theme
  • solved stacking context layout issue Sidebar menu space issue #1614
  • accessibility wcag 2.1 guidelines
  • reconstruct content layout design

toc added on following pages

mobile light mode mobile dark mode desktop
Screenshot 2025-02-21 033329 Screenshot 2025-02-21 033439 Screenshot 2025-02-21 033556

Resolves #1760, #1614, #1807

Edits

To stop flashing on tap, -webkit-tap-highlight-color : transparent is applied. Header mobile dropdown menus might need :focus, :active class selectors. This chrome specific issue.

before after
XRecorder_20250221_01_001 XRecorder_20250221_02_001-ezgif com-video-to-gif-converter

@ShubhamOulkar ShubhamOulkar requested review from a team as code owners February 7, 2025 08:54
Copy link

netlify bot commented Feb 7, 2025

Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit e1fed96
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/67c38e103517430008f024ea
😎 Deploy Preview https://deploy-preview-1775--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR.

  • Please don't increase the font size in this PR, that's a separate issue that requires a dedicated PR, which distracts from the main focus of this one.
  • Also, keep the original style for desktop for now. The design you propose looks great for mobile, though some styles need to be adjusted.
  • Please don't format the code in areas that are outside the scope of this PR.

Most importantly, we should find a way for Jekyll to generate that table. Moving it to another file might not be the best approach since I'm working on making the API translatable in Crowdin, and managing translations in a separate file would be more complicated.

I'd even be open to using JavaScript to generate the table.

These are my main comments for now. I'll try to review it more thoroughly tomorrow or the day after, but I'm really tired today.

@chrisdel101
Copy link
Contributor

chrisdel101 commented Feb 8, 2025

Hi @ShubhamOulkar, Thanks for the PR. I was just looking at this and have some concerns.

  • The menu file should not be removed like this. It seems to me that this is already essentially a kind of TOC for the API pages, but for larger screens only.
  • So you'd really be adding this to screens where the menu is not visible, and mobile. I like the design.
  • So the TOC menu should operate and display the dropdowns in the same way as the current menu does. So when you click it opens the sublinks.
  • The current JS can likely be reused for that. It might be possible to just use the same menu or copy and rejig it - up to you. Seems unnecessary to rewrite this, unless that was part of the original issue.
  • why only on api V4x? If it's a simple includes I see no reason not to add everywhere it's going to be used.
  • As Sebastian says please remove all changes that are only formatting.

@ShubhamOulkar
Copy link
Member Author

@bjohansebas, Thank for your kind suggestions. Now it is ready for review.

  1. accessibility improvements removed: font size and line spacing removed.
  2. removed 4x api menu data .yml file.

@ShubhamOulkar ShubhamOulkar changed the title Add Table of Contents Component for Express 4.x API Documentation page Add Table of Contents Component for api pages Feb 9, 2025
@bjohansebas
Copy link
Member

This button should be visible on smaller screens, around 800px, or when the layout actually breaks, not on 1200px screens.
Image

@bjohansebas
Copy link
Member

It shouldn't create that element
image

@ShubhamOulkar
Copy link
Member Author

ShubhamOulkar commented Feb 12, 2025

Thank you @bjohansebas @chrisdel101
@bjohansebas removed observer api as you suggested.
@chrisdel101 I adapted to code logic in my recent commits please review it.
Let me know if we can do it more better.

@bjohansebas
Copy link
Member

I don't think we should display that button yet when scrolling, or we should integrate it better. At first glance, it's not clear what the button does after scrolling.

image

By the way, fix the conflicts.

@bjohansebas
Copy link
Member

bjohansebas commented Feb 12, 2025

An example of how it could be better integrated into the page
Close:
image

Open:
image

@ShubhamOulkar
Copy link
Member Author

ShubhamOulkar commented Feb 12, 2025

An example of how it could be better integrated into the page

I think you didn't check max-width=540px break point. small screen width integrated btn appears below header. This floating btn appears in between 540 to 800 wide screens on scroll. Actually I don't like that arrow, I am searching proper icon for this. Right now only btn title appears on the screen.

@ShubhamOulkar ShubhamOulkar changed the title Add Table of Contents Component for api pages Feat : Add Table of Contents Component on pages Feb 12, 2025
@ShubhamOulkar
Copy link
Member Author

ShubhamOulkar commented Feb 22, 2025

@chrisdel101 , layout redesign is not for placing side bar on right or left. We could do with old positioning used in css code just by changing position right value. Layout redesign happen because of the issue #1614 . Previously all components placed manually used positioning. So it gets hard to solve issue #1614 by using js. So I suggested @bjohansebas to redesign layout css using flexbox and removed positioning, unnecessary css classes. Now we can reuse all side bar html on all pages. This is main reason to redesign layout css not placing right or left. Placing right or left is teams choice. I prefer to think which side is more accessible? Who is user? How most of the user read? It is confusing and complex to think.

About our discussion, I am user of this site for years. I navigated on api pages by using ctrl+f🤣. I really got annoyed. I have tested api pages and are really handy with right sidebar menus. About blogs pages right sidebar are annoyingly hard to read because menus are long with line breaks and words spacing. On blogs pages we can put it on left just because they are navigating to another page. And it is standard position on most websites.

We must talk about what design we want? Then only we are able to integrate modern design. Now I did with minimalistic approach with existing styles and improved accessibility.

@chrisdel101
Copy link
Contributor

chrisdel101 commented Feb 22, 2025

I prefer the left since I find if easier to read, think navigation pane in Word, and since it seems your functionality would work on either side, but I'm not requesting you change it. Seem Sebastian is okay with right side too and you had your reasons to put it on the right.

Yeah I used ctrl-f too. Hopefully this helps us not use that as much ha.

@ShubhamOulkar
Copy link
Member Author

ShubhamOulkar commented Feb 26, 2025

👋 @bjohansebas and @chrisdel101, Is it necessary to add underlines in sidebar menus? I prefer to add underlines only in docs because we need to distinguish links from the text content. And adding underlines or outlines on user interaction for sidebar menus.

Note: some documentation websites use underlines only on links(sidebar links blogs, middleware's) that are navigate to another page. And links that are navigating on same page don't use underlines (api pages, changelogs').

Now we are using standard design for sidebar menus, same page navigation menu's on right and another page navigation menus on left. I want know ur thoughts. Right now I keep underline and outline for testing. Which one should be removed?

@bjohansebas
Copy link
Member

Good question, I think we could remove the underline in the TOC.

some documentation websites use underlines only on links(sidebar links blogs, middleware's) that are navigate to another page. And links that are navigating on same page don't use underlines (api pages, changelogs').

That's a good idea, although I think we don't have any links pointing to other websites, at least in the menus.

@ShubhamOulkar
Copy link
Member Author

ShubhamOulkar commented Feb 27, 2025

@bjohansebas, TOC is ready to launch 🚀.

note : fixed #1807 because it is relevant to this pr.

@bjohansebas
Copy link
Member

bjohansebas commented Feb 27, 2025

Great, I will review it properly very soon.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, There's still work to be done to fix the layout, but it will be in another PR. Thanks for the hard work!

@bjohansebas bjohansebas merged commit 2cda09d into expressjs:gh-pages Mar 1, 2025
8 checks passed
@ShubhamOulkar ShubhamOulkar deleted the table-of-content branch March 2, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add table of contents
3 participants